Fix issue #1898 - Permit::release() must be called when Mutex::lock() is aborted even if it is in the Waiting state #1902

bikeshedder · 2019-12-05T09:32:45Z

It is possible to leave a mutex in a locked state even if no future exists holding the lock.

create Mutex
lock Mutex
try to lock mutex in another future that is aborted (e.g. via timeout)
unlock Mutex
locking the mutex again fails

I think this should be considered a critical issue as it can be exploited as shown in issue #1898 when using hyper and aborting requests.

bikeshedder · 2019-12-05T11:32:25Z

Permit::release must be called regardless if it's in the state Acquired or Waiting. If a Waiting Permit is not released its waiter will stay in the queue blocking any future Permit to be acquired.

The original code did the following:

release semaphore if it is aquired
panic via unreachable! if the semaphore is idle or waiting unless the code is already panicking

Since a Permit must always be released unless it is in the Idle state I wonder if that unreachable! is even needed. If needed the following code should be added at the beginning of MutexGuard::drop:

if self.permit.state == semaphore::PermitState::Idle && !::std::thread::panicking() {
    unreachable!("Permit was idle when MutexGuard was dropped")
}

Permit::release() must also be called when it is in the waiting state.

hawkw

This looks good to me, but it would be good to hear from @carllerche as well. I commented on some minor nits.

hawkw · 2019-12-05T17:55:08Z

tokio/tests/sync_mutex.rs

+            iv.tick().await;
+            iv.tick().await;


The use of the interval here is a little un-obvious, it could be good if there was a comment explaining what it's doing here?

I thought that the comment Try to lock mutex in a future that is aborted prematurely was telling enough. tbh. That test case is not really needed. I just added it when debugging the code and trying to reproduce the locking behaviour. This test just makes sure Aquired locks are returned when their future is aborted.

Is there a better way to let a future delay its execution than creating an interval and waiting twice? tick().await is called twice as the first call of it returns instantly.

Is there a better way to let a future delay its execution than creating an interval and waiting twice? tick().await is called twice as the first call of it returns instantly.

You could possibly just use

task::yield_now().await;

if all you want is to yield to the scheduler so that another task will be polled? But, what you're doing now does make sense, I just thought it would be helpful if there was a comment explaining what it was for.

I'm going to move forward merging & shipping. We can tweak the test in a follow up PR.

tokio/src/sync/mutex.rs

hawkw · 2019-12-05T17:59:34Z

tokio/tests/sync_mutex.rs

+#[tokio::main]
+#[test]
+/// Ensure a mutex is unlocked if a future holding the lock
+/// is aborted prematurely.


Nit: It would be good to reference the GitHub issue that these tests reproduce.

carllerche

Thanks for debugging & fixing 👍

bikeshedder force-pushed the feature/mutex-aborted-future branch from 519fd4c to c13853c Compare December 5, 2019 09:42

bikeshedder changed the title ~~Add tests for mutex issue #1898~~ Fix issue #1898 - possible deadlock when future waiting for a semaphore is aborted Dec 5, 2019

Add tests for mutex issue tokio-rs#1898

e5debd6

bikeshedder force-pushed the feature/mutex-aborted-future branch from 48de2b3 to 472c87b Compare December 5, 2019 11:34

Fix tokio-rs#1898

257a904

Permit::release() must also be called when it is in the waiting state.

bikeshedder force-pushed the feature/mutex-aborted-future branch from 472c87b to 257a904 Compare December 5, 2019 11:37

bikeshedder changed the title ~~Fix issue #1898 - possible deadlock when future waiting for a semaphore is aborted~~ Fix issue #1898 - Permit::release() must be called when Mutex::lock() is aborted even if it is in the Waiting state Dec 5, 2019

hawkw approved these changes Dec 5, 2019

View reviewed changes

carllerche requested a review from jonhoo December 5, 2019 18:37

carllerche approved these changes Dec 6, 2019

View reviewed changes

carllerche merged commit c632337 into tokio-rs:master Dec 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #1898 - Permit::release() must be called when Mutex::lock() is aborted even if it is in the Waiting state #1902

Fix issue #1898 - Permit::release() must be called when Mutex::lock() is aborted even if it is in the Waiting state #1902

bikeshedder commented Dec 5, 2019 •

edited

Loading

bikeshedder commented Dec 5, 2019

hawkw left a comment

hawkw Dec 5, 2019

bikeshedder Dec 5, 2019

hawkw Dec 5, 2019

carllerche Dec 6, 2019

hawkw Dec 5, 2019

carllerche left a comment

Fix issue #1898 - Permit::release() must be called when Mutex::lock() is aborted even if it is in the Waiting state #1902

Fix issue #1898 - Permit::release() must be called when Mutex::lock() is aborted even if it is in the Waiting state #1902

Conversation

bikeshedder commented Dec 5, 2019 • edited Loading

bikeshedder commented Dec 5, 2019

hawkw left a comment

Choose a reason for hiding this comment

hawkw Dec 5, 2019

Choose a reason for hiding this comment

bikeshedder Dec 5, 2019

Choose a reason for hiding this comment

hawkw Dec 5, 2019

Choose a reason for hiding this comment

carllerche Dec 6, 2019

Choose a reason for hiding this comment

hawkw Dec 5, 2019

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

bikeshedder commented Dec 5, 2019 •

edited

Loading